Create hls-export-plugin with Export '...' code action#4952
Conversation
crtschin
force-pushed
the
crtschin/export-1-export
branch
3 times, most recently
from
418edd5 to
e9a5ab3
Compare
|
I've tried this PR with GHC for #4967. This PR indeed fixes the invalid syntax problem, but unfortunately it does expand CPP macros in the export list. That file uses this macro in its export list: |
Thanks for testing this out! That's an interesting wrinkle, I believe HLS generally works with post-preprocessor sources. So I'd have to build something to specifically work with preprocessor directives. I won't do that here, as the PR is already quite large, but I'll note it in #4948. |
For the curious, I now have a patch locally for this, but I'll refrain from making a PR until this is merged, as there isn't a nice way to do stacked PRs on GH (yet). |
fendor
left a comment
fendor
left a comment
There was a problem hiding this comment.
Awesome patch, thank you very much! I have a couple of small notes and suggestions.
| @@ -0,0 +1,283 @@ | |||
| {-# LANGUAGE CPP #-} | |||
There was a problem hiding this comment.
Parts of this module might fit well into hls-exactprint-utils as well
There was a problem hiding this comment.
I'll defer this until other plugins start touching the export list. These would have only one consumer in the export plugin.
| #if MIN_VERSION_ghc(9,8,0) | ||
| isUnusedTopBind d = | ||
| case d ^? fdStructuredMessageL . _SomeStructuredMessage . msgEnvelopeErrorL . _TcRnMessage of | ||
| Just (TcRnUnusedName _ UnusedNameTopDecl) -> True | ||
| _ -> False | ||
| #else | ||
| isUnusedTopBind _ = False | ||
| #endif |
There was a problem hiding this comment.
This CPP could be avoided (or pushed to another place) by introducing a pattern synonym for TcRnUnusedName in Compat.Error. In that pattern synonym, you use the CPP to make sure it can't be mached on older GHC versions.
Edit: Or introduce another prism _TcRnUnusedName that won't ever match on older GHC versions
| runExport :: (FilePath -> Session a) -> IO a | ||
| runExport act = | ||
| runSessionWithTestConfig def |
There was a problem hiding this comment.
Personally, I like pushing the testCase label into the plugin specific test runner, e.g.
| runExport :: (FilePath -> Session a) -> IO a | |
| runExport act = | |
| runSessionWithTestConfig def | |
| runExport :: String -> (FilePath -> Session a) -> IO a | |
| runExport label act = | |
| testCase label $ runSessionWithTestConfig def |
But that's just my opinion, this is fine as well.
| @@ -0,0 +1,15 @@ | |||
| cradle: | |||
There was a problem hiding this comment.
Such a cradle can lead to flaky tests as the test session compiles all test modules in each test. Waiting for progress done, or waiting for a specific diagnostic, can sometimes then be signalled from the wrong source file.
Not a blocker, but generally we prefer to use the test helpers that copy the sources individually into temporary directories for better isolation.
Also, this layout might have issues with your work on #4364, as they would be sharing build directories.
A shared library lets the export plugin reuse them without depending on hls-refactor-plugin.
Offered on `-Wunused-top-binds` diagnostics.
The hls-export-plugin Export action replaces it.
Freshly built names carry EpAnnNotUsed before GHC 9.9, so addParens could not parenthesize operators.
TcRnUnusedName gained structured provenance only in GHC 9.8, so 9.6 cannot identify unused top-level binds to attach the action to.
crtschin
force-pushed
the
crtschin/export-1-export
branch
from
e9a5ab3 to
d498988
Compare
3 participants
First part to #4948.
This differs from the existing action that
hls-refactor-pluginprovided:hls-refactor-pluginonly worked when the symbol was detected unused.